-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't build the rust-demangler
binary for coverage tests
#125816
Conversation
The coverage-dump tool already needs `rustc_demangle` for its own purposes, so the amount of extra code needed for a demangle mode is very small.
This avoids the need to build `rust-demangler` when running coverage tests, since we typically need to build `coverage-dump` anyway.
This appears to be the canonical way to build a tool with the stage 0 compiler.
Some changes occurred in src/tools/compiletest cc @jieyouxu |
This looks like mostly a T-bootstrap change |
One of the reasons I want to do this is that the We might be able to follow up by getting rid of that binary entirely, since the need for demangling seems to be well-served by |
Is the rust-demangler something we ship with releases or can we now rip it out? |
I honestly don't know, and I was in the middle of trying to figure out who to even ask. There does seem to be code in bootstrap for including it in source/binary tarballs, but I have no idea where it ends up, and so far I haven't seen any evidence of anyone actually using it. |
FWIW, the README says
But IDK how accurate that is now. |
It's not shipped by default in nightly (or other releases). It may be in a component I don't have, but I can't think of which one should have it. |
ok @bors r+ let's MCP removing the tool separately just for visibility (in the hope if anyone still knows a need for it, they'll speak up) |
Don't build the `rust-demangler` binary for coverage tests The coverage-run tests invoke `llvm-cov`, which requires us to specify a command-line demangler that it can use to demangle Rust symbol names. Historically this used `src/tools/rust-demangler`, which means that we currently build two different command-line tools to help with the coverage tests (`rust-demangler` and `coverage-dump`). However, it occurred to me that if we add a demangler mode to `coverage-dump` (which is only a handful of lines and no extra dependencies), then we only need to build one helper binary for the coverage tests, and there is no need for tests to build `rust-demangler` at all. --- Note that the `rust-demangler` binary is separate from the `rustc-demangle` crate (which both `rust-demangler` and `coverage-dump` use as a dependency to do the actual demangling). --- So the main benefits/motivations here are: - Slightly faster builds after a fresh checkout or bootstrap bump. - Making it clear that currently no tests actually need the `rust-demangler` binary, since the coverage tests can use their own tool instead.
Rollup of 6 pull requests Successful merges: - rust-lang#125652 (Revert propagation of drop-live information from Polonius) - rust-lang#125730 (Apply `x clippy --fix` and `x fmt` on Rustc) - rust-lang#125752 (run-make: enforce `#[must_use]` and arm command wrappers with drop bombs) - rust-lang#125756 (coverage: Optionally instrument the RHS of lazy logical operators) - rust-lang#125796 (Also InstSimplify `&raw*`) - rust-lang#125816 (Don't build the `rust-demangler` binary for coverage tests) r? `@ghost` `@rustbot` modify labels: rollup
Don't build the `rust-demangler` binary for coverage tests The coverage-run tests invoke `llvm-cov`, which requires us to specify a command-line demangler that it can use to demangle Rust symbol names. Historically this used `src/tools/rust-demangler`, which means that we currently build two different command-line tools to help with the coverage tests (`rust-demangler` and `coverage-dump`). However, it occurred to me that if we add a demangler mode to `coverage-dump` (which is only a handful of lines and no extra dependencies), then we only need to build one helper binary for the coverage tests, and there is no need for tests to build `rust-demangler` at all. --- Note that the `rust-demangler` binary is separate from the `rustc-demangle` crate (which both `rust-demangler` and `coverage-dump` use as a dependency to do the actual demangling). --- So the main benefits/motivations here are: - Slightly faster builds after a fresh checkout or bootstrap bump. - Making it clear that currently no tests actually need the `rust-demangler` binary, since the coverage tests can use their own tool instead.
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#125652 (Revert propagation of drop-live information from Polonius) - rust-lang#125730 (Apply `x clippy --fix` and `x fmt` on Rustc) - rust-lang#125756 (coverage: Optionally instrument the RHS of lazy logical operators) - rust-lang#125776 (Stop using `translate_args` in the new solver) - rust-lang#125796 (Also InstSimplify `&raw*`) - rust-lang#125807 (Also resolve the type of constants, even if we already turned it into an error constant) - rust-lang#125816 (Don't build the `rust-demangler` binary for coverage tests) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#125816 - Zalathar:demangler, r=oli-obk Don't build the `rust-demangler` binary for coverage tests The coverage-run tests invoke `llvm-cov`, which requires us to specify a command-line demangler that it can use to demangle Rust symbol names. Historically this used `src/tools/rust-demangler`, which means that we currently build two different command-line tools to help with the coverage tests (`rust-demangler` and `coverage-dump`). However, it occurred to me that if we add a demangler mode to `coverage-dump` (which is only a handful of lines and no extra dependencies), then we only need to build one helper binary for the coverage tests, and there is no need for tests to build `rust-demangler` at all. --- Note that the `rust-demangler` binary is separate from the `rustc-demangle` crate (which both `rust-demangler` and `coverage-dump` use as a dependency to do the actual demangling). --- So the main benefits/motivations here are: - Slightly faster builds after a fresh checkout or bootstrap bump. - Making it clear that currently no tests actually need the `rust-demangler` binary, since the coverage tests can use their own tool instead.
Remove `src/tools/rust-demangler` `rust-demangler` is a small binary that reads a list of mangled symbols from stdin, demangles them (using the `rustc-demangle` library crate), and prints the demangled symbols to stdout. It was added as part of the initial implementation of coverage instrumentation in 2020/2021, so that coverage tests could pass it to `llvm-cov --Xdemangler` when generating coverage reports. It has been largely untouched since then. As of rust-lang#125816 it is no longer used by coverage tests, and has no remaining in-tree uses. There is code in bootstrap to build and package the demangler, but it's unclear where the resulting binaries actually end up, or whether there's any reasonable way for `rustup` users to obtain them. --- For users needing a command-line demangler, `rustfilt` exists and is more actively maintained. It's also quite easy to use the `rustc-demangle` library to build a custom command-line demangler if necessary, with only a few lines of code. The tool's name (`rust-demangler`) is easily confused with the name of the library crate `rustc-demangle`, so removing the tool will eliminate that confusion. There also doesn't appear to be much reason to use `rust-demangler` over `rustfilt`. --- This PR therefore removes the tool, and removes all of its associated code from bootstrap. MCP filed: rust-lang/compiler-team#754
Rollup merge of rust-lang#125880 - Zalathar:demangler, r=oli-obk Remove `src/tools/rust-demangler` `rust-demangler` is a small binary that reads a list of mangled symbols from stdin, demangles them (using the `rustc-demangle` library crate), and prints the demangled symbols to stdout. It was added as part of the initial implementation of coverage instrumentation in 2020/2021, so that coverage tests could pass it to `llvm-cov --Xdemangler` when generating coverage reports. It has been largely untouched since then. As of rust-lang#125816 it is no longer used by coverage tests, and has no remaining in-tree uses. There is code in bootstrap to build and package the demangler, but it's unclear where the resulting binaries actually end up, or whether there's any reasonable way for `rustup` users to obtain them. --- For users needing a command-line demangler, `rustfilt` exists and is more actively maintained. It's also quite easy to use the `rustc-demangle` library to build a custom command-line demangler if necessary, with only a few lines of code. The tool's name (`rust-demangler`) is easily confused with the name of the library crate `rustc-demangle`, so removing the tool will eliminate that confusion. There also doesn't appear to be much reason to use `rust-demangler` over `rustfilt`. --- This PR therefore removes the tool, and removes all of its associated code from bootstrap. MCP filed: rust-lang/compiler-team#754
The coverage-run tests invoke
llvm-cov
, which requires us to specify a command-line demangler that it can use to demangle Rust symbol names.Historically this used
src/tools/rust-demangler
, which means that we currently build two different command-line tools to help with the coverage tests (rust-demangler
andcoverage-dump
).However, it occurred to me that if we add a demangler mode to
coverage-dump
(which is only a handful of lines and no extra dependencies), then we only need to build one helper binary for the coverage tests, and there is no need for tests to buildrust-demangler
at all.Note that the
rust-demangler
binary is separate from therustc-demangle
crate (which bothrust-demangler
andcoverage-dump
use as a dependency to do the actual demangling).So the main benefits/motivations here are:
rust-demangler
binary, since the coverage tests can use their own tool instead.